Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use CrossVersion.for3Use2_13 in Scala.js #1054

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JD557
Copy link
Contributor

@JD557 JD557 commented Feb 25, 2024

Fix #1024

This seems to be enough and, from my tests, both code compiled with Scala 3 and Scala 2.13 now works as expected.

I'm not sure if there's anything that I could have done to allow one to optionally use CrossVersion.for3Use2_13, but one can always just compile with 2.13 if that's the case.

Copy link
Collaborator

@rochala rochala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big kudos for the contribution! I had only one small comment.
The CI is down, so don't worry about it failing. After you make the change, I'll merge and release it.

@@ -31,7 +31,7 @@ object SbtShared {
val sbt = latest212
val jvm = latest213
val cross = List(latest210, latest211, latest212, latest213, old3, js, sbt, jvm).distinct
val crossJS = List(latest212, latest213, js).distinct
val crossJS = List(latest212, latest213, stableLTS, js).distinct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be old3. The reason is that Scastie can run all Scala versions and 3.0.0 is the one that we can use in all newer versions. If we use stableLTS, we won't be able to use versions below it.

Copy link
Contributor Author

@JD557 JD557 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. I missed that (I thought you could only pick LTS and Next).

I do get an error compiling apiJS3 when I change that:

[error] error while loading $throws$package$,
[error] class file scala/runtime/$throws$package.class is broken, reading aborted with class dotty.tools.tasty.UnpickleException
[error] TASTy signature has wrong version.
[error]  expected: {majorVersion: 28, minorVersion: 0}
[error]  found   : {majorVersion: 28, minorVersion: 1}
[error]
[error] This TASTy file was produced by a more recent, forwards incompatible release.
[error] To read this TASTy file, please upgrade your tooling.
[error] The TASTy file was produced by Scala 3.1.3-bin-nonbootstrapped

I'll see if I can find out what's going on.

Copy link
Collaborator

@rochala rochala Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I know the reason. So this is caused because the Scala version is lower than some library on classpath.

So the fix has to be something different. The exact cause why we need this cross compilation is for our runtime macros that we use in worksheet to display value decorations. The error you see comes from play json dependency.

If I may suggest something:
Leave the versions 2.12, 2.13 and js.
Then we should only handle the runtime-scala dependency to always cross compile:

"org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT" cross CrossVersion.for3Use2_13

and handle the others in the way you've changed it to work.

Copy link
Contributor Author

@JD557 JD557 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that initially, but that causes another problem: runtime-scala includes scalajs-dom, so in this case it would pull the 2.13 version

So any Scala 3 library that includes scalajs-dom then fails. :(

Maybe one alternative would be to use "org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT" cross CrossVersion.for3Use2_13 if the scala target is lower than 3.3.0 and "org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT" otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fix for that in my Scala-CLI PR. I dropped play json and it should work. Until then I think there is nothing we can do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wasn't aware of #997, that looks pretty exciting.

I'll eagerly wait for that, then 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scala JS always uses CrossVersion.for3Use2_13
2 participants